Skip to content

Changing toggle styles to use flex box#4313

Merged
phkuo merged 6 commits intomicrosoft:masterfrom
jdrush89:jdrush89/Change-Toggle-Styling-To-Use-Flexbox
Mar 20, 2018
Merged

Changing toggle styles to use flex box#4313
phkuo merged 6 commits intomicrosoft:masterfrom
jdrush89:jdrush89/Change-Toggle-Styling-To-Use-Flexbox

Conversation

@jdrush89
Copy link
Copy Markdown
Contributor

@jdrush89 jdrush89 commented Mar 19, 2018

Pull request checklist

Description of changes

Removing the absolute positioning on the toggle thumb and using flexbox positioning instead to keep the toggle aligned correctly for all text sizes.

Focus areas to test

Test that toggles render correctly for multiple text sizes

@jdrush89 jdrush89 requested a review from phkuo as a code owner March 19, 2018 19:25
Copy link
Copy Markdown
Contributor

@phkuo phkuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! We definitely prefer flexbox over absolute positioning.

You'll need to update the test snapshot with your style changes though: src/components/Toggle/Toggle.test.tsx
Run npm run update-snapshots in the packages\office-ui-fabric-react folder.

Copy link
Copy Markdown
Member

@betrue-final-final betrue-final-final left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you accidentally show the toggle as "off" instead of "on" when "on disabled".
image

@jdrush89
Copy link
Copy Markdown
Contributor Author

@betrue-final-final It looks like screener was not picking up my latest commit for whatever reason. Now it's complaining that the toggle has changed, though running npm run update-snapshots doesn't update anything at this point. Any ideas?

@betrue-final-final
Copy link
Copy Markdown
Member

Not sure. I'll let the screener guy know.

@betrue-final-final
Copy link
Copy Markdown
Member

Did you check your snap tests?

@betrue-final-final
Copy link
Copy Markdown
Member

Looks like whatever you did fixed it!

@jdrush89
Copy link
Copy Markdown
Contributor Author

@phkuo snapshots are updated, let me know if it looks good

padding-left: .2em;
padding-right: .2em;
padding-top: 0;
position: relative;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'm surprised this didn't get removed

@phkuo phkuo merged commit 2527a28 into microsoft:master Mar 20, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Toggle thumb is misaligned when overriding text size

3 participants